Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_kubernetes_cluster - gate node_os_channel_upgrade check since it's a preview feature #22284

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

stephybun
Copy link
Member

Due to the nature of the feature we had to add in some fairly stringent validation to prevent users from ending up with config that diverged from state.

But the validation currently also prevents clusters from being created even if the preview feature isn't enabled. This rearranges the validation to not interfere with users not wishing to use the preview feature.

@stephybun stephybun marked this pull request as ready for review June 26, 2023 14:07
@stevehipwell
Copy link

@stephybun according to the last AKS release notes it's only the SecurityPatch node upgrade channel which is behind the preview flag.

@stephybun
Copy link
Member Author

@stevehipwell yes I saw this in the docs and unfortunately that behaviour is not what I observed for the version that we're currently on which is 2023-04-02-preview. I got errors from the API that the preview feature isn't registered when trying to set node_os_upgrade_channel to None or Unmanaged as well.

From the release notes it looks like that behaviour applies from the versions 2023-06-01 and 2023-06-02-preview onwards which I don't see in the Azure REST API repo yet. Once they're available in the repo we can look into upgrading the API version and confirming whether that behaviour is valid.

@stevehipwell
Copy link

@stephybun setting NodeImage, which is the default, seems fine without the feature gate set?

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stephybun - LGTM 👍

@stephybun
Copy link
Member Author

@stevehipwell setting default values on fields is problematic in AKS since they aren't usually returned by the API until they've been explicitly set by the user. This is compounded by the fact that this is also a preview feature so the default value could change from version to version which would then constitute a breaking change in the provider.

This feature is actually a great example of that since the msft docs currently states

By default, a cluster's node OS auto-upgrade channel is set to Unmanaged.

But the changelog says

Default for node os upgrade channel updated to NodeImage in 2023-06-01 and 2023-06-02-preview APIs.

Since we cannot move to either of those versions yet, we believe this fix is the least disruptive way to expose it until we're able to upgrade the API version in the provider.

@github-actions github-actions bot added size/S and removed size/XS labels Jun 28, 2023
@stephybun stephybun merged commit ed8e0b6 into main Jun 28, 2023
@stephybun stephybun deleted the b/aks-move-node-os-check branch June 28, 2023 10:46
@github-actions github-actions bot added this to the v3.63.0 milestone Jun 28, 2023
stephybun added a commit that referenced this pull request Jun 28, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants